fix(ci): report override keys too narrow to cover a vulnerable copy - #2174
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c3061acdf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e79e393972
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
React Doctor skipped this pull request — it changed no React files. Reviewed by React Doctor for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9ba4fe57a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 661e3d3368
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
661e3d3 to
c549817
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5498176aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Yosemite-Crew/scripts/ci/check-override-advisories.mjs
Lines 596 to 597 in 1108212
When an accepted uncovered-copy entry stops matching, the documented entry shape has no pinned field, so this cleanup message prints identifiers such as uuid@undefined. Format this kind using its uncovered versions (or at least its kind and package) so maintainers can identify the stale acceptance they are being asked to delete.
AGENTS.md reference: AGENTS.md:L33-L33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The guard only ever asked "is any override PIN stale". It never asked "does any override KEY fail to COVER a vulnerable copy", and that second question is the one that mattered for uuid: the pins were a patched 11.1.1 under the exact keys uuid@11.1.0 and uuid@9.0.1, while the vulnerable copies resolved at 7.0.3, 8.0.0 and 8.3.2 via xcode, aws-sdk and sockjs. Neither key matches anything in the 7.x or 8.x lines, so the override never applied to them and six high alerts sat open while this check reported OK. Its own test suite recorded that as intended, in a case commented "the override is not the thing at fault". The override was the thing at fault. findVulnerablePins now emits a second kind of finding, uncovered-copy, next to the existing stale-pin, and suggests the range key that would actually cover the escaping versions. Run against origin/dev with real pnpm audit data it reports the nanoid stale pin exactly as before and additionally derives "uuid@<11.1.1": "11.1.1", which is the fix #2173 arrived at by hand. Deciding coverage needs the key compared as a semver range, so the file carries a small comparator rather than gaining a dependency; it still imports only node: builtins. Supported: no selector, bare major, major.minor, exact, < <= > >= =, ^ and ~, and x-ranges. Caret, tilde and x-range are handled rather than left to the catch-all because that direction is a false negative: a "uuid@8.x" key silently treated as covering a 7.0.3 copy would be the same blind spot one syntax along. A version equal to a pinned value is excluded from the uncovered set. An override rewrites resolution TO its pin, so an exact key never selects its own target: "vite@7.3.3": "7.3.5" installs 7.3.5, which vite@7.3.3 does not match. Without that exclusion every ordinary stale pin also reported as an uncovered copy, with prose contradicting the stale-pin finding beside it, on 24 of the 78 overridden packages here. Simulating a fresh advisory on nine of those real pinned targets now yields 11 stale pins and 0 uncovered copies. Baseline entries stay backwards compatible: existing records have no kind field and still match, and an uncovered-copy can be accepted by an entry carrying kind plus uncovered, so the escape hatch covers both. check:overrides is deliberately still not wired into CI. It exits 1 on origin/dev today, for the nanoid pin (#2172) and the uuid keys (#2173), so wiring it before those land would red the pipeline immediately. The new tests are gated regardless, since _core.yaml runs test:scripts. 102 tests pass. Every new test was verified to fail against the unfixed logic.
Six defects from review on the first version of this change, all real: Parent-scoped keys claimed to cover copies under any parent. splitOverrideKey discarded the parent, leaving the child selector-less, so it read as covering everything. Live here: react-native>jest-environment-node has no blanket sibling, so every jest-environment-node in the tree looked covered. Coverage now checks the parent against the advisory's dependency paths, and credits the key only when no path information exists. Partial comparator bounds were zero-padded rather than expanded. semver reads '>1' as "from 2.0.0" and '<=1.2' as "all of 1.2.x"; padding got both backwards, calling 1.5.0 covered by '>1' and 1.2.5 uncovered by '<=1.2'. An ordinary selector matched a prerelease, so an exact pkg@1.2.3 key was read as covering 1.2.3-alpha.1. semver and pnpm both exclude it, so the override would not actually apply and a vulnerable prerelease could pass unreported. x-ranges fell through to the catch-all, so uuid@8.x was treated as covering a 7.0.3 copy. uuid@8 and uuid@8.x are one intent; handling only the first left the same blind spot one syntax along. Suggested keys were derived from the first version token of patched_versions. For a disjoint range like '<2.0.0 || >=2.0.5' that produced "<2.0.0": "2.0.0", which misses a vulnerable 2.0.3 and pins copies outside the patched set. A suggestion is now offered only for a simple lower-bounded range. Baseline keys joined the uncovered versions in audit order, so an accepted entry went stale the day pnpm listed them differently. Sorted now. The baseline file also documented only the stale-pin shape, so following it for an uncovered-copy produced an entry that could never match; both shapes are now documented with examples. 109 tests pass. Each fix was verified to fail the suite when reverted. The real uuid case is still caught and still suggests "uuid@<11.1.1": "11.1.1", and simulating fresh advisories on nine real escaping pinned targets still yields 11 stale pins and 0 uncovered copies.
An exact prerelease selector matched nothing above it and fell to the
permissive fallback, so a key like pkg@1.2.3-alpha.1 read as covering every
version in the tree. Measured: selectorCovers('1.2.3-alpha.1', '9.9.9') was
true. It is a point and is now compared as one.
Parent-scoped coverage used paths.some, so one matching path credited the key
for the whole version. The same version can arrive both under the scoped
parent and elsewhere, and the occurrence the override cannot rewrite was then
suppressed. Every path must go through the parent now.
The success line claimed every override pins a patched version whenever
nothing was unaccepted, which is also true when findings are merely baselined.
That contradicted the accepted-drift block printed directly above it and read
as falsely reassuring. It now reports no unreviewed findings and says how many
accepted entries are still vulnerable.
81 tests pass, 109 across test:scripts. Each fix verified to fail when reverted.
Five more from review, two of them mistakes in the previous round's fix: Coverage of a version is "for EVERY path, SOME key covers that path". Folding the `every` into overrideKeyCovers inverted that, so two sibling keys like foo>child and bar>child each failed on the other's path and the pair was reported as leaving an uncovered copy it actually covers. Split into a per-path predicate with the version-level quantifiers in versionIsCovered. Parent matching compared substrings, so key foo>child was credited for the path 'app > foobar > child@1.0.0'. Segments are now compared by package identity, with or without a version suffix. The prerelease gate only ran in the bare/x-range branch, so pkg@<2.0.0 and pkg@^1.0.0 still claimed to cover 1.2.3-alpha.1. semver keeps a prerelease out of every ordinary range, so the check moved to the top of selectorCovers and a selector that names a prerelease is the documented exception. A patched range of '>1.2.3' means the fix is ABOVE 1.2.3, but the suggestion regex accepted it and fixedVersionFrom then proposed pinning to the still vulnerable 1.2.3. Only '>=' names a pastable release now. 'pkg@>v1.2.3' and 'pkg@> 1.2.3' were read as parent>child, indexing the entry under 'v1.2.3' and skipping every advisory for pkg. A '>' preceded by '@', '<' or '>' is always an operator. 87 tests pass, 117 across test:scripts. The real uuid case is still caught with the same suggested key, and the nine real escaping pinned targets still produce zero uncovered copies.
Review found four more defects in the hand-rolled comparator, on top of the four in the previous round. Every one was a silent false negative in a security check: - a caret or tilde selector carrying a prerelease matched no branch and fell to the permissive fallback, so pkg@^1.2.3-alpha.1 covered every version - prerelease identifiers were compared lexicographically, so 1.0.0-alpha.10 sorted below 1.0.0-alpha.2 and <1.0.0-alpha.2 claimed to cover alpha.10 - a compound range like >=1 <2 was waved through as covering everything - partial comparator bounds and prerelease eligibility each needed their own special case, and each was wrong before it was fixed Eight defects across four rounds is the approach failing, not bad luck. This swaps the whole comparator for semver, which is what pnpm resolves with. The dependency-free constraint turned out to be self-imposed rather than a repo rule: scripts/ci already imports istanbul-lib-* for coverage. Also fixes two findings semver does not cover: - a pnpm parent selector overrides the matched parent's OWN dependency, so foo>child cannot reach a child that an intermediate package depends on. The path check now requires the parent to sit directly before the child rather than anywhere in the ancestry. - the uncovered-copy remediation text claimed the pin was patched and promised a suggested key even when none was printed, which happens for a no-fix, disjoint or exclusively bounded patched range. It is now conditional, and the no-suggestion case says to read the advisory instead. 91 tests in this file, 121 across test:scripts. Behaviour on the real data is unchanged: the uuid case is still caught with the same suggested key, and the nine real escaping pinned targets still produce zero uncovered copies.
semver.coerce was used only as a truthiness guard before validating the original strings, so it never changed the outcome and made the comparison harder to read. Validate directly instead. Unorderable input still sorts equal rather than throwing, since one malformed audit version should not abort the whole check.
… header Three more from review. A version-scoped parent key was reduced to its bare name, so `foo@1>child` was credited for a path through foo@2.0.0. pnpm applies such a key only when the parent's own version satisfies the selector, so a patched pin could suppress a copy the override never rewrites. The path check now tests the parent's version too, and passes when the path carries no version to compare. The parent separator was told apart from the range operator by the character AFTER the '>', which rejected any child whose name starts with a digit. `2fa` is a real package name, and `foo>2fa` was indexed under the literal string rather than the child, skipping every advisory for it. The operator is identified by the character before the '>' instead, which is always one of @ < > or =. The header still described the hand-rolled comparator, the dependency-free constraint and compound and x-ranges being unrecognised. The previous commit replaced all of that with semver, so the documented contract was wrong about the check's own false-negative policy. Rewritten to match. 93 tests here, 123 across test:scripts. Each fix verified to fail the suite when reverted.
19363d8 to
c8d2fe3
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
|
|
|



PR Checklist
On the middle box: this came out of reviewing #2173 rather than from a filed issue, and it is a change to a CI script rather than a security report, so there is nothing to link.
What is the current behavior?
scripts/ci/check-override-advisories.mjsexists to catch override pins that have gone stale, and it does that job: run onorigin/devit exits 1 and correctly reportsnanoidpinned at 3.3.17 against GHSA-2v37-7h3g-55p8.It only ever asks one question though, "is any override PIN stale". It never asks the other one, "does any override KEY fail to COVER a vulnerable copy". The check is:
exacttests the pinned value, so for an exact pin the guard fires only when the tree resolved to the pinned version itself.That is why the uuid problem in #2173 survived. The pins were a patched
11.1.1under the exact keysuuid@11.1.0anduuid@9.0.1, while the copies that were actually vulnerable resolved at 7.0.3, 8.0.0 and 8.3.2 through xcode, aws-sdk and sockjs. Neither key matches anything in the 7.x or 8.x lines, so the override never applied to those copies, nothing equalled11.1.1, and the guard stayed silent while Grype reported six high alerts.The test suite recorded that as deliberate, in a case named "ignores a vulnerable installed copy that the override does not pin", commented:
The override was the thing at fault. Its key was too narrow to reach the copies that needed patching.
What is the new behavior?
findVulnerablePinsnow produces two kinds of finding.stale-pinis unchanged.uncovered-copyis new: for a package that has overrides, any vulnerable installed copy that no override key selects is reported, together with the range key that would cover it.Run against
origin/devwith realpnpm auditdata, it reports the nanoid stale pin exactly as before and adds:The suggested key is the fix #2173 arrived at by hand, derived independently.
Range handling, and why it is hand-rolled
Answering the coverage question means comparing the override key as a semver range. The file is dependency-free by design and still imports only
node:builtins, so it carries a small comparator instead of taking a dependency.Supported selector shapes: no selector, bare major (
3), major.minor (3.3), exact (9.0.1), the<<=>>==comparators,^and~ranges, and x-ranges (8.x,8.*,8.x.x).Anything else, such as a compound range or a workspace protocol, is treated as covering the version. That errs towards silence rather than a false alarm, and the stale-pin check still watches those keys.
^,~and x-ranges are handled rather than left to that catch-all on purpose. The catch-all direction is a false negative, and auuid@8.xkey silently treated as covering a 7.0.3 copy would be the same blind spot one syntax along.^0.xfollows npm's left-most-non-zero rule, so^0.2.3covers 0.2.9 but not 0.3.0.Not double-reporting an ordinary stale pin
A version equal to one of the pinned values is excluded from the uncovered set before anything is reported.
This matters more than it sounds. An override rewrites resolution to its pinned value, so an exact-version key never selects its own target:
"vite@7.3.3": "7.3.5"installs 7.3.5, which the keyvite@7.3.3does not match. Without the exclusion, the day an advisory lands on a pinned target the guard emits the correct stale-pin AND an uncovered-copy for the same version, whose remediation prose ("the pinned value is already patched but the override KEY is too narrow") directly contradicts the finding printed beside it.That is not an edge case here: 24 of the 78 overridden packages have a pinned value that escapes every one of their own key selectors, including glob, jws, path-to-regexp, esbuild, form-data, joi and vite. Simulating a fresh advisory against nine of those real pinned targets now yields 11 stale pins and 0 uncovered copies.
Baseline
Backwards compatible. Existing entries carry no
kindfield and still match a stale-pin finding. An uncovered-copy can be accepted by an entry carryingkindplusuncovered, keyed on the uncovered versions so it expires the same way the pinned-version key does: cover one of them, or let a new copy appear, and the entry stops matching. Both were verified with fixtures.On wiring check:overrides into CI
Deliberately not done here.
check:overridesis currently only apackage.jsonscript and is referenced by no workflow, so it gates nothing. Wiring it in now would red the pipeline immediately, because it exits 1 onorigin/devfor two reasons that are already fixed elsewhere and not yet merged: the nanoid pin (#2172) and the uuid keys (#2173). Turning it on before those land would either break every PR or invite someone to baseline two live findings just to get green, which is exactly the routing-around this script's own comments warn about.It should be wired in as a follow-up once both merge, at which point it exits 0. The new logic is not unguarded in the meantime:
_core.yamlline 85 runspnpm run test:scripts, so all 72 tests in this file are CI-gated.Validation
node --test scripts/ci/check-override-advisories.test.mjsgives 72 pass, 0 failpnpm run test:scriptsgives 102 pass, 0 fail across the ci and security script suitesnode:builtins--strictwith unreachable advisory data, 0 for non-strict unreachable, 0 for--helpEvery new test was verified to fail against the unfixed logic, by reverting each behaviour in turn and confirming exactly the expected cases go red: removing the uncovered-copy detection, breaking the caret upper bound, treating tilde as caret, dropping x-range handling, and dropping the pinned-value exclusion.
Two of the findings fixed above, the double-report and the x-range gap, came from an adversarial review of the first version of this change rather than from writing it.
Related Issue(s)
Follows #2173 (uuid range-key coverage), which is the incident this gap allowed, and #2172 (nanoid override bump). Both need to merge before
check:overridescan be wired into CI.